feat(in-app-messaging): add BannerMessage UI component #9124

calebpollman · 2021-10-28T16:02:41Z

Description of changes

Add In-App Messaging BannerMessage React Native UI component for rendering InAppMessage payloads created in the Amazon Pinpoint console:

Example Top Banner

with square image, header, some payload style

with square image and primaryButton

iOS screen shot only
with square image, header, some payload style (container backgroundColor and message text color) and override style (close icon color, message fontSize)

Example Middle Banner

with landscape image, header, primaryButton

Example Bottom Banner

with header, primaryButton, secondaryButton, some payload style

Issue #, if available

NA

Description of how you validated changes

Manually tested in sample app

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

elorzafe

LGTM, just a couple non-blocking questions 🌮

elorzafe · 2021-10-28T17:19:08Z

packages/aws-amplify-react-native/src/InAppMessaging/components/constants.ts

+
+// TODO: delete these once they are no longer needed
+// pinpoint max chars 150
+export const MAX_MESSAGE =


Why MAX_MESSAGE instead of MAX_MESSAGE_LENGTH?

No real reason, they are just for my testing

packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/BannerMessage.tsx

elorzafe · 2021-10-28T17:53:46Z

packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/BannerMessage.tsx

+					{primaryButton && (
+						<View style={styles.buttonsContainer}>
+							{secondaryButton && (


What happen if there is only secondaryButton?

Ideally, the contract we have with pinpoint should prevent that from happening, but it seems reasonable to add handling for that scenario

packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/BannerMessage.tsx

packages/aws-amplify-react-native/src/InAppMessaging/components/MessageWrapper/index.ts

cshfang · 2021-10-28T19:40:10Z

packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/BannerMessage.tsx

+
+	return shouldDelayMessageRendering ? null : (
+		<MessageWrapper>
+			<View style={[styles.positionContainer, styles[position]]}>


Nit: This is not blocking but I wonder if styles should be defaultStyle or something? I keep finding myself having to remind myself what the difference between styles and style is. As a reader of foreign code, I would have no idea why they're different things.

imo default styling as styles is a pretty standard convention in React Native. That said i do think all the styling applied in these components is not the easiest to parse 😟

default styling as styles is a pretty standard convention in React Native

This is a good point too since static StyleSheets are typically assigned to a styles variable. It was just a thought

cshfang · 2021-10-28T19:49:21Z

packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/styles.ts

+	buttonText: {
+		fontSize: BASE_FONT_SIZE,
+		fontWeight: BASE_FONT_WEIGHT,
+		lineHeight: getLineHeight(BASE_FONT_SIZE),


Nit: You could extract this to ../constants as well so that it doesn't need to be recalculated if it needs to be used elsewhere.

cshfang · 2021-10-28T19:53:36Z

packages/aws-amplify-react-native/src/InAppMessaging/components/constants.ts

+export const SMALL_SPACING = 4;
+export const BASE_SPACING = 8;
+export const LARGE_SPACING = 12;
+export const EXTRA_LARGE_SPACING = 16;


Nit: Consider switching this around - i.e. SPACING_X so developers can easily access spacing values rather than typing LARGE and then having to choose spacing vs fontsize etc. This could even be a SPACING object that contains different size properties.

Agree with naming, but prefer to keep these explicit exports. Basically feel like putting them in an object implies they should be long lived instead of temporary

cshfang · 2021-10-28T20:06:39Z

packages/aws-amplify-react-native/src/InAppMessaging/components/utils.ts

+
+import { LINE_HEIGHT_MULTIPLIER } from './constants';
+
+export const getLineHeight = (fontSize: number) => fontSize * LINE_HEIGHT_MULTIPLIER;


Nit: Should this utility be less tied to LINE_HEIGHT_MULTIPLIER i.e. accept multiplier as a parameter? Just a nit question since I don't see any reason why the multiplier would change right now (though it could in theory be configurable in the future)

So between facebook/react-native#29507 and my experience with building components in RN feel it is kind of negligent to allow the values of lineHeight to be anything but 1.5 the size of the accompanying fontSize. I'm sure there could still be an argument for allowing it to be configurable, but just wanted to put that out there

cshfang · 2021-10-28T20:08:28Z

packages/aws-amplify-react-native/src/InAppMessaging/hooks/useInAppMessage/useInAppMessage.ts

-	useInAppMessaging,
-} from '../..';
+import { BannerMessageProps, CarouselMessageProps, FullScreenMessageProps } from '../../components';
+import { InAppMessageComponents, useInAppMessaging } from '../../context';


Nit: Should useInAppMessaging be in hooks? Isn't it a little unintuitive that this one hook lives elsewhere?

I'm kind of leaning towards moving that hooks directory under the components directory since the contents are not going to be public exports, and they have very specific use cases related to the In-App Messaging components. Personally think useInAppMessaging should stay close to the InAppMessagingContext since that is what it exposes, but could be swayed to move it

I feel it would be more intuitive to simply have hooks live together. I know it exposes the context, but it's not really the context itself but rather a helper/wrapper around useContext right?

cshfang · 2021-10-28T20:10:30Z

packages/aws-amplify-react-native/src/icons/index.ts

+	close: require('./close.png') as ImageSourcePropType,
+	warning: require('./warning.png') as ImageSourcePropType,


Nit: Are these assets accounting for high pixel density displays? (They look pretty huge so I'm guessing it's fine)

For my own understanding, are SVG's supported on RN?

@reesscot Not out of the box, but with external dependencies yes

cshfang

Lgtm

codecov-commenter · 2021-10-28T23:03:26Z

Codecov Report

Merging #9124 (01128d8) into in-app-messaging/main (64a4090) will not change coverage.
The diff coverage is n/a.

@@                  Coverage Diff                   @@
##           in-app-messaging/main    #9124   +/-   ##
======================================================
  Coverage                  78.00%   78.00%           
======================================================
  Files                        250      250           
  Lines                      18115    18115           
  Branches                    3885     3885           
======================================================
  Hits                       14131    14131           
  Misses                      3854     3854           
  Partials                     130      130

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64a4090...01128d8. Read the comment docs.

nickarocho

LGTM! Just a small nit comment for naming conventions.

nickarocho · 2021-10-28T20:00:24Z

packages/aws-amplify-react-native/src/InAppMessaging/components/BannerMessage/types.ts

+	bottom: ViewStyle;
+	middle: ViewStyle;
+	top: ViewStyle;
+


I like the grouping approach you did here :)

nickarocho · 2021-10-28T20:01:59Z

packages/aws-amplify-react-native/src/InAppMessaging/components/constants.ts

+// color
+export const BLACK = '#000';
+export const LIGHT_GREY = '#e8e8e8';
+export const WHITE = '#fff';


Nit: I'm not yet familiar with the React Native codebase's patterns, but in the future, it could be good to prefix the color consts with COLOR_ - i.e. COLOR_BLACK, COLOR_WHITE, COLOR_GREY_LIGHT (similar to BANNER_) in order to group the declarations closer together.

github-actions · 2022-10-30T02:15:44Z

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

feat(in-app-messaging): add BannerMessage UI component

436e683

calebpollman requested a review from Ashish-Nanda as a code owner October 28, 2021 16:02

calebpollman requested review from cshfang, elorzafe, ericclemmons and reesscot and removed request for Ashish-Nanda October 28, 2021 16:05

elorzafe approved these changes Oct 28, 2021

View reviewed changes

cshfang reviewed Oct 28, 2021

View reviewed changes

address feedback

01128d8

cshfang approved these changes Oct 28, 2021

View reviewed changes

reesscot approved these changes Oct 28, 2021

View reviewed changes

calebpollman merged commit 8a6bd99 into aws-amplify:in-app-messaging/main Oct 28, 2021

calebpollman deleted the in-app-messaging/add-banner-message branch October 28, 2021 23:15

nickarocho approved these changes Oct 29, 2021

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(in-app-messaging): add BannerMessage UI component #9124

feat(in-app-messaging): add BannerMessage UI component #9124

calebpollman commented Oct 28, 2021

elorzafe left a comment

elorzafe Oct 28, 2021

calebpollman Oct 28, 2021

elorzafe Oct 28, 2021

calebpollman Oct 28, 2021

cshfang Oct 28, 2021

calebpollman Oct 28, 2021 •

edited

Loading

cshfang Oct 28, 2021

cshfang Oct 28, 2021

cshfang Oct 28, 2021

calebpollman Oct 28, 2021 •

edited

Loading

cshfang Oct 28, 2021

calebpollman Oct 28, 2021

cshfang Oct 28, 2021

calebpollman Oct 28, 2021

cshfang Oct 28, 2021

cshfang Oct 28, 2021

reesscot Oct 28, 2021

calebpollman Oct 28, 2021

cshfang left a comment

codecov-commenter commented Oct 28, 2021

nickarocho left a comment

nickarocho Oct 28, 2021

nickarocho Oct 28, 2021

github-actions bot commented Oct 30, 2022


		import { LINE_HEIGHT_MULTIPLIER } from './constants';

		export const getLineHeight = (fontSize: number) => fontSize * LINE_HEIGHT_MULTIPLIER;

		close: require('./close.png') as ImageSourcePropType,
		warning: require('./warning.png') as ImageSourcePropType,

feat(in-app-messaging): add BannerMessage UI component #9124

feat(in-app-messaging): add BannerMessage UI component #9124

Conversation

calebpollman commented Oct 28, 2021

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

elorzafe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebpollman Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebpollman Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshfang left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 28, 2021

Codecov Report

nickarocho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2022

calebpollman Oct 28, 2021 •

edited

Loading

calebpollman Oct 28, 2021 •

edited

Loading